You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A macOS ACL can grant write or rename access without changing Unix mode bits. Trust storage previously accepted such paths, allowing an existing user-owned approval marker to be renamed to the key for changed command definitions while retaining its valid canonical-config-path contents.
This change rejects write-capable ACLs on trust-store ancestors, the final directory, and existing markers. It shares the established Git-configuration ACL policy with the CLI and generated Bash/Zsh/Fish helpers, fails closed when ACL inspection fails, and advances the shell-integration cache schema to 14.
Motivation
Approval must remain bound to the command definitions the user reviewed. Read-only and deny ACLs, safe config-home symlinks, relative paths, and supported existing storage modes remain usable. The validation does not repair permissions or modify existing approvals.
Type of Change
Bug fix
Code refactoring: share the existing ACL policy
Testing
Reproduced the failure against the original implementation at 9f99add: a renamed approval allowed the unreviewed hook to create its output file, and the generated Bash helper accepted ACL-writable storage. The regression tests pass after this fix.
All 910 BATS tests passed on macOS with system Bash 3.2, run by file with isolated Git and XDG settings.
The CI ShellCheck command passed.
./scripts/generate-completions.sh --check and git diff --check passed.
Native macOS ACL coverage includes ancestors, markers, nested symlink paths, read-only and deny ACLs, inspection failures, and generated Bash/Zsh/Fish helpers. Platform-dependent tests are gated on Darwin.
Manual Testing Checklist
Tested on:
macOS, Bash 3.2
Linux — not run locally
Windows (Git Bash) — not run locally
Core functionality tested:
git gtr new, including --porcelain
git gtr rm
Interactive git gtr trust
Generated Bash, Zsh, and Fish trust helpers
Both production and development entry points
Test Steps
In a disposable repository with temporary trust storage:
Approve through the actual git gtr trust terminal prompt; create a worktree and verify its approved hook runs.
Change the hook, grant a write-capable macOS ACL, and rename the original approval marker to the changed definitions' key; verify worktree creation skips the hook and reports skipped-untrusted.
Attempt approval through git gtr trust; verify it rejects unsafe storage without modifying the marker or ACL.
Remove the unsafe ACL and replayed marker, explicitly approve again, and verify the changed hook runs.
Remove the disposable worktrees.
Expected and actual behavior matched. The marker rename was simulated under the same OS account; a second-user attack was not executed. Safe inherited ACLs were also smoke-tested.
Breaking Changes
No new commands, dependencies, or configuration keys. Storage with write-capable macOS ACLs is now rejected; safe storage remains supported.
Checklist
Read CONTRIBUTING.md and followed the project style
Added regression coverage and performed manual testing
Noted platform-specific behavior and local validation limits
Generated completions remain current; no command or flag changes
No actionable comments were generated in the recent review.
Walkthrough
The change rejects write-capable macOS ACLs on hook trust-storage paths and existing approval markers. CLI and generated Bash, Zsh, and Fish helpers use the shared ACL checker. The init cache schema advances to 14, with regression tests for ACL validation and renamed approval markers.
Changes
Hook trust storage
Layer / File(s)
Summary
Shared ACL policy lib/config_safety.sh
Adds _config_safety_acl_check and uses it for config-node validation. The helper skips ACL inspection outside Darwin, rejects write-capable ACLs, and reports inspection failures.
CLI and generated helper validation lib/hooks.sh, lib/commands/init.sh
CLI and generated helpers check ACLs on trust-storage roots, traversed directories, final directories, and existing markers. The generated cache schema advances to 14.
Trust and cache regression tests tests/trust_storage.bats, tests/init.bats
Adds coverage for ACL conditions, ACL inspection failures, renamed approval markers, generated shell helpers, and schema 14 cache updates.
sequenceDiagram
participant EntryPoint as CLI or generated shell helper
participant TrustPath as _hooks_safe_trust_path
participant ACLCheck as _config_safety_acl_check
participant LS as ls
EntryPoint->>TrustPath: validate trust path
TrustPath->>ACLCheck: check root, directories, and marker ACLs
ACLCheck->>LS: inspect ACL with ls -lde
LS-->>ACLCheck: ACL entries or inspection failure
ACLCheck-->>TrustPath: pass or failure reason
TrustPath-->>EntryPoint: trusted or rejected
Loading
Possibly related PRs
coderabbitai/git-worktree-runner#230: Adds ownership, mode, and file-type validation for hook trust storage, which this change extends with ACL checks.
Suggested reviewers: natoboram
Merge Risk: Minimal · up to 0170c
No actionable issue remains from this review; the change is mergeable after normal checks.
Security architecture risk: Low · up to 0170c
The change strengthens approval checks without expanding execution authority. No introduced security regression was identified in the reviewed paths. Native macOS behavior was not independently exercised, and already-running shells need refreshed integration to receive the fix.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The affected authority is command execution as the invoking user. A compromised shared trust directory can affect approvals for multiple repositories using that user's storage; the reviewed change does not introduce remote-service, tenant, or privileged deployment authority.
Security Findings and Attack Paths
inferred — The addressed attack combines changed repository command definitions with ACL-enabled approval-marker renaming. A renamed, user-owned marker can retain valid configuration-path contents despite a different definition key. The head's directory ACL gate rejects that storage before marker acceptance; regression tests encode this denial for CLI and generated helpers. This assessment is source-based, not an independently executed exploit verification.
Trust Boundaries and Controls
observed — The new ACL checks supplement, rather than replace, ownership, mode, regular-file, symlink, and approval-identity controls. ACL inspection errors return failure, and trust-storage callers propagate that failure instead of treating an unknown result as approval.
Resilience and Maintainability Implications
inferred — Trust checks inspect live storage repeatedly rather than caching ACL acceptance. Validation remains point-in-time: subsequent reads and publication use pathnames without binding the checked filesystem identity to use. That limitation predates the PR, and the comparison shows no new caller or authority expansion worsening it.
Pre-merge checks: 5
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title clearly describes the main change: rejecting unsafe macOS ACLs on hook trust storage.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 5 files.
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the trust-store gate, For ACLs that can change a marker’s state. Read-only paths may still pass through, While unsafe grants are turned away too. Schema fourteen hops into view.
Reviewing files that changed from the base of the PR and between 88a31de and 0170ccc.
Files selected for processing: 5
lib/commands/init.sh
lib/config_safety.sh
lib/hooks.sh
tests/init.bats
tests/trust_storage.bats
Included review availability: This review used your included allowance. Your plan provides up to 100 included reviews per hour; 99 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Description
A macOS ACL can grant write or rename access without changing Unix mode bits. Trust storage previously accepted such paths, allowing an existing user-owned approval marker to be renamed to the key for changed command definitions while retaining its valid canonical-config-path contents.
This change rejects write-capable ACLs on trust-store ancestors, the final directory, and existing markers. It shares the established Git-configuration ACL policy with the CLI and generated Bash/Zsh/Fish helpers, fails closed when ACL inspection fails, and advances the shell-integration cache schema to 14.
Motivation
Approval must remain bound to the command definitions the user reviewed. Read-only and deny ACLs, safe config-home symlinks, relative paths, and supported existing storage modes remain usable. The validation does not repair permissions or modify existing approvals.
Type of Change
Testing
9f99add: a renamed approval allowed the unreviewed hook to create its output file, and the generated Bash helper accepted ACL-writable storage. The regression tests pass after this fix../scripts/generate-completions.sh --checkandgit diff --checkpassed.Manual Testing Checklist
Tested on:
Core functionality tested:
git gtr new, including--porcelaingit gtr rmgit gtr trustTest Steps
In a disposable repository with temporary trust storage:
git gtr trustterminal prompt; create a worktree and verify its approved hook runs.skipped-untrusted.git gtr trust; verify it rejects unsafe storage without modifying the marker or ACL.Expected and actual behavior matched. The marker rename was simulated under the same OS account; a second-user attack was not executed. Safe inherited ACLs were also smoke-tested.
Breaking Changes
No new commands, dependencies, or configuration keys. Storage with write-capable macOS ACLs is now rejected; safe storage remains supported.
Checklist
CONTRIBUTING.mdand followed the project styleLicense Acknowledgment
This contribution is made under the terms of the Apache License 2.0.
Summary by CodeRabbit